-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate new public key in Account #989
Validate new public key in Account #989
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good, Eric! I left a couple questions on the proposed message hash construction and a few minor suggestions
let message_hash = PoseidonTrait::new() | ||
.update_with('StarkNet Message') | ||
.update_with('accept_ownership') | ||
.update_with(get_contract_address()) | ||
.update_with(current_owner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we include the domain separator in compliance with SNIP-12?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And I assume you don't think it's worth it to include the account and the new owner in the message as Marto initially proposed?
#[derive(Copy, Drop, Serde)]
struct AddOwner {
account: ContractAddress,
owner: felt252
}
Even though the account is already present in the hash as required by SNIP-12 and that owner is already known by the owner, I decided to make the struct overly explicit so users know what they're signing, even if it adds a bit of cost/redundancy/complexity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this particular case, I don't think SNIP12 is suited, because we don't really need to sign a struct with nonces and so on, we just need to verify that the signer exists, and for that something like EIP 191 (version 0x45) should suffice. If we were to be SNIP 12 compliant, we would need to add a domain separator, with SNIP12Metadata, and that seems like an overhead for the use case.
The proposed format is to avoid clashes with transactions by prefixing "StarkNet Message", and accept_ownership to scope the message, and then we have the account address and the previous owner, so the user is signing who will be receiving the ownership from. To me adding the new owner to the signature is redundant since the signature already enforces the new pub key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that, the use case is to avoid transferring the ownership to an invalid public key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
something like EIP 191 (version 0x45) should suffice. If we were to be SNIP 12 compliant, we would need to add a domain separator, with SNIP12Metadata, and that seems like an overhead for the use case.
To me adding the new owner to the signature is redundant since the signature already enforces the new pub key.
Good points!
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
Co-authored-by: Andrew Fleming <[email protected]>
…lo/cairo-contracts into feat/validate-new-pub-key-#818
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Fixes #818
PR Checklist